Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
endigo9740
left a comment
endigo9740
left a comment
There was a problem hiding this comment.
@Hugos68 Overall everything looks great. Just a few suggestions here. Let me know if you have any questions or feedback. Thanks bud!
| display: grid; | ||
| gap: --spacing(2); | ||
| } | ||
| &[data-part='frame'] { |
There was a problem hiding this comment.
Make sure to include an explicit example of modifying the color of the frame/pattern parts, as they require different and specific properties.
Maybe brand color for the frame, brand contrast for the pattern.
| <QrCode.Frame> | ||
| <QrCode.Pattern /> | ||
| </QrCode.Frame> | ||
| <QrCode.Overlay>Overlay</QrCode.Overlay> |
There was a problem hiding this comment.
Follow Zag's lead on demoing the overlay feature, just use a Lucide icon + square background styling to help separate from the busy pattern. All in user-land of course.
Don't follow their lead on the poor choice of color contrast though :D
| color: var(--color-surface-100-900); | ||
| } | ||
| &[data-part='download-trigger'] { | ||
| @apply btn preset-filled; |
There was a problem hiding this comment.
I was meaning to talk to you about the use of presets in our components. While I think btn is fine for adding form, I think presets may have gotchas. The reason being is preset classes don't "swap" like the used to, they override.
If you were to apply apply class="preset-outlined-primary-500" on this button as a user, you would expect a transparent background with a border edge of the colors defined. But what you get instead is a filled button with an outline added.
One of two things needs to happen:
- We need to prune presets from the stylesheet and keep those in user-land only. A breaking change, but my recommendation.
- We need to add an implicit
bg-transparenton the outline presets.
I'd have real concern for number 2 as it may unintentionally break this pattern:
preset-tonal preset-outlined-surface-200-800
This would result in a tonal fill but outlined edge. Aka what we used to call "ghost" styling in v2. But if we add an explicit transparent bg, it may render this unviable.
| <QrCode.Pattern /> | ||
| </QrCode.Frame> | ||
| <QrCode.Overlay>Overlay</QrCode.Overlay> | ||
| <QrCode.DownloadTrigger mimeType="image/png" fileName="skeleton-qr-code">Download</QrCode.DownloadTrigger> |
There was a problem hiding this comment.
Just a consideration for another example - folks might request a version where you can click on the QR code to download it. Maybe try a variation with the frame/pattern wrapped by this button?
| - Namespace composition uses `Object.assign(Root, { ...parts })` in `modules/anatomy.ts`. | ||
| - For static parts, include `data-scope` and `data-part` attributes. | ||
|
|
||
| ## Prompt flow |
There was a problem hiding this comment.
This looks awesome. Only bit of feedback is perhaps prompt users for a Zag documentation URL. Then notify the LLM here how to convert to the machine-friendly version:
- From: https://zagjs.com/components/{framework}/{component}
- To: https://zagjs.com/api/mdx/components/{framework}/{component}
You might even try and include the Github source for the component using a similar pattern, so it can have a deep level of understanding of how this operates.
Just to make this an explicit part of the user<->llm transaction upfront.
|
|
||
| 1. **Component name** — kebab-case folder and PascalCase namespace. Recommend both, accept overrides. | ||
| 2. **Type** — static/simple or machine-backed (Zag). This resolves required module files. | ||
| 3. **Parts list** — root-only or full anatomy list (`root`, `trigger`, `content`, etc.). |
There was a problem hiding this comment.
Step 3 and 4 feels like something the LLM could likely suggest for us if provided the documentation mentioned above. Just accept user overrides.
| - Svelte suite: `packages/skeleton-svelte/test/components/<component>.test.ts` | ||
|
|
||
| 4. For each anatomy part, include a render assertion (`toBeInTheDocument`). | ||
| 5. If the component should be previewable in the playgrounds, add matching demo entries in both frameworks and regenerate the React route manifest if a new route was added. |
There was a problem hiding this comment.
Nothing prompts the user if playground should be created, so might be worth being a default on, user opt-out?
|
|
||
| Ask one at a time, confirm each answer, write nothing until all are answered and the user confirms. | ||
|
|
||
| 1. **Component slug** - kebab-case route segment (for example `floating-panel`). |
There was a problem hiding this comment.
When folks are using LLMs they're going to likely use natural language and write "Floating Panel" or "floating panel" or " I want to add a floating panel component". So allow any input, but let the LLM coerce to specific kabab format here. Again user confirmation is good.
Same deal for the other skill btw.
2 participants
Linked Issue
Closes #{issueNumber}
Description
Adds:
AI Disclosure
Use of LLM technology is allowed. We ask for your voluntary disclosure to help inform future Skeleton contribution guidelines.
Checklist
Please read and apply all contribution requirements.
docs/,feature/,task/,bugfix/mainbranchpnpm checkin the root of the monorepopnpm formatin the root of the monorepopnpm lintin the root of the monorepopnpm testin the root of the monorepo/packageprojects, please supply a ChangesetChangesets
View our documentation to learn more about Changesets. To create a Changeset:
pnpm changesetand follow the prompts